Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: remove null URNs from census data #1759

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

PsypherPunk
Copy link
Collaborator

Context

Minor follow-up to #1748 to tidy some inconsistencies seen in the output data.

AB#242989

Change proposed in this pull request

  • explicitly drop any null entries from the URN data before setting the index
  • simplify the joining of pupil and workforce data (join will default to using the index)

Guidance to review

.dropna(subset=["URN"])

The workforce data contains a null row for some of the earlier years. Previously, this was stripped when the inner join didn't find a match with the pupil data. However, following #1688, the outer join keeps this in place resulting in an index that is a mix of integers (i.e. URNs) and floats (the null is expressed as a float).

removal of on="URN",

join() defaults to using the index: we want to explicitly join in the index and by this point, can guarantee that we want to join both datasets on their respective indexes.

Checklist (add/remove as appropriate)

  • Work items have been linked (use AB#)
  • Your code builds clean without any errors or warnings
  • You have run all unit/integration tests and they pass
  • Your branch has been rebased onto main
  • You have tested by running locally
  • You have reviewed with UX/Design

- explicitly drop any null entries from the URN data before setting the
  index
- simplify the joining of pupil and workforce data (`join` will default
  to using the index)
Copy link
Collaborator

@Faizan-ahmad00 Faizan-ahmad00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants